@dechamps - Thanks for doing this. Handling quirks in the underlying host APIs is an important feature for PortAudio.

This change was tested ...

@dechamps please could you update the PR description with information about which OS version(s) and audio devices were used for your testing. This gives us traceability if someone wants to compare your results later.

Based on my testing reported at #775, I can get stable bidirectional audio with a 5.3ms input buffer size no problems with standard Realtek motherboard sound. I do get stalling with low output buffer sizes, so maybe there is a bug, but I don't think we have characterized it correctly yet. In any case with the available information, I don't think it's appropriate to put a blanket clamp to 31ms. I think we should continue the root-cause analysis at #775.

If it were the case that there are no systems where lower input buffer size works, or there was a way to detect that we were on such a system where the buffer limit applied, then the clamp would make total sense.

While I'm all for quirk workarounds, my take is that there will always be "low" latency/buffer settings which will produce unreliable output on some or all systems with some or all host APIs. It's up to the end user to tweak the buffering for best performance on their system. And that includes discovering the system limits. In other words, we should allow for parameter tuning beyond functioning limits. This PR prevents that.

If your goal is to provide a "foolproof" configuration that never fails due to quirks of some devices (by clamping even if it degrades latency on some system), then that logic should be put in the application code. (i.e. the application implements the clamp). PortAudio already provides some infrastructure to support this in PaDeviceInfo:

It might make sense to update these default latency values to reflect the current findings especially for the non-interactive case.

Based on my testing reported at #775, I can get stable bidirectional audio with a 5.3ms input buffer size no problems with standard Realtek motherboard sound. I do get stalling with low output buffer sizes, so maybe there is a bug, but I don't think we have characterized it correctly yet. In any case with the available information, I don't think it's appropriate to put a blanket clamp to 31ms. I think we should continue the root-cause analysis at #775.

I just commented on #775 to show you that you did precisely reproduce the problem I characterized. Given the various indications that this is a widespread problem, and no evidence that there exists systems that aren't affected, I would say the blanket clamp is appropriate.

If it were the case that there are no systems where lower input buffer size works

It certainly looks like it. And you can add your own system to that list since you did reproduce it.

It's up to the end user to tweak the buffering for best performance on their system. And that includes discovering the system limits. In other words, we should allow for parameter tuning beyond functioning limits. This PR prevents that.

If your goal is to provide a "foolproof" configuration that never fails due to quirks of some devices (by clamping even if it degrades latency on some system), then that logic should be put in the application code.

If this failure mode merely resulted in glitchy audio, then I would agree (for example this is why I closed #788 instead of trying to implement a clamp in MME). However the failure mode we're dealing with here is much worse - it doesn't just cause glitchy audio, instead it results in a stall which is impossible to recover from and can easily compromise the stability of the application (deadlock), forcing the user to kill the app and restart it. I don't think this is acceptable behaviour from any library under any circumstances, regardless of parameters.

Isn't there somebody you forgot to ask?
(cue XP, especially given that's presumably the major use case for dsound)

Isn't there somebody you forgot to ask?
(cue XP, especially given that's presumably the major use case for dsound)

Do you have evidence that #775 does not apply to XP? If so, I would be happy to add some kind of version check for that.

Also, to be clear: even if this new code runs on systems that are not subject to #775, that still doesn't mean these systems would break. The worst that can happen is that there might be some increase to the minimum achievable DS input latency, and even that is not certain (it depends on various parameters and on how DS relates input read cursor granularity to buffer size).

On the other hand, #775 does break modern systems in a pretty bad way.

(cue XP, especially given that's presumably the major use case for dsound)

My project does not even support XP, yet it uses dsound by default. This is because, when that decision was made, dsound looked like the most "modern" PortAudio Windows Host API that seemed reliable enough (WASAPI had lots of problems back then, and I suspect it still does, but I need to look into it).

I don't think it's fair to assume that users are using DS primarily to be compatible with XP. If a user cares deeply about compatibility with old platforms, then I would expect them to use MME which is the default and is more reliable and more widely supported than DS.

Do you have evidence that #775 does not apply to XP?

Not really.
But it would still seem almost impossible tbf?
It's not just that some functions may have been rewritten (or abstracted to wasapi), in your usual XP machine a lot of of the processing was up to the audio driver itself.
So even going from a realtek to an nforce to a xfi could give you pretty different "low level" results.

I don't think it's fair to assume that users are using DS primarily to be compatible with XP.

My judgement was just theoretical, idk about history or implementation quirks.
Of course not all shortcomings may matter to certain use case, but it seems pretty obvious that it is a deprecated second-tier api.

Do you have evidence that #775 does not apply to XP?

Not really.
But it would still seem almost impossible tbf?

Okay, fair enough. Given that #775 affecting pre-Vista Windows seems unlikely, and given that the buffer size clamp could plausibly hurt low-latency DS users who are still on XP, I have updated this PR to do a version check so that it is a no-op on pre-Vista Windows.

I think all concerns with this PR have been addressed now?

My personal status: I want to do more local testing before I comment further.

We are considering this PR for the next release, milestone 19.8.
We need to do a more through review and resume the conversation.

As I understand it, #772 is blocking this, and I will look into that one.

As I understand it, #772 is blocking this, and I will look into that one.

No, this PR does not depend on #772. #772 is only required if you want to test this PR using paloopback. This is because paloopback shows DirectSound as glitchy - regardless of this PR - due to #770, and #772 is the fix for that.

Is there a reason, why this priority 1 PR get not merged?

Is there a reason, why this priority 1 PR get not merged?

It is stuck waiting for me to have time to review it properly.

This is my interim review, awaiting discussion with Phil. TLDR: for now no changes are requested. Phil and I need to decide whether to merge more-or-less as-is or require a different approach that doesn't add latency in the input+output case.

clamp input buffer size at 62.5 ms

In fact the current PR clamps both input and output buffer sizes to 62.5ms, except for output-only streams, where the buffer size follows the old calculations. The change is limited to Vista and later where the workaround is understood to be required.

I will take it at face value that it is necessary to clamp the input buffer size on some systems. I have not independently verified this. I have not encountered it myself, nor had it reported as a bug against my applications.

The substantive content of this PR is to add three lines to the end of CalculateBufferSettings()

    if( hasInput && PaWinUtil_GetOsVersion() >= paOsVersionWindowsVistaServer2008 )
    {
        *hostBufferSizeFrames = max( *hostBufferSizeFrames, 2 * 0.03125 * sampleRate );
    }

By performing this clamping in CalculateBufferSettings() the output buffer size will also be clamped (in the input+output case). I expect this will increase output latency beyond what is strictly necessary. Obviously that is undesirable. (Also obviously, without some kind of fix our code is not working as-is.)

Our implementation assumes the same buffer size for input and output, and I am not sure how easy it would be to adjust it to use different buffer sizes for input and output. Certainly the current PR seems like a safe approach whereas clamping only the input buffer size (to be different from the output buffer size) requires more careful analysis for possible problems.

this PR includes a small refactoring of the DirectSound buffer calculation code which was giving me headaches as I was working on the fix.

As far as I'm concerned this PR includes a complete rewrite of the logic in CalculateBufferSettings(). The rewrite turns out to perform exactly the same calculations as the old code. Convincing myself of this fact was time I could have better spent elsewhere. Please be mindful that the ease of getting things merged is inversely proportional to the time it takes code review to verify that nothing was broken.

I acknowledge that the existing code could be improved. For the purposes of this patch my preference is probably to keep the current code and just add the clamping patch at the end of the function. But let's not get into that now. No changes requested.

Conclusion: this is one of the few blockers for the 19.8 release and I think #775 is severe enough to warrant an imperfect fix. Ideally this fix would not have the side effect of adding latency in the input+output case, but that would most likely be a bigger and riskier change.

Phil and I will discuss this at our next review meeting (most likely morning February 21 Melbourne time).